Skip to content

improve: naming reconcile pre-condition is just condition #2306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

improve: naming reconcile pre-condition is just condition #2306

wants to merge 3 commits into from

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Mar 21, 2024

No description provided.

csviri added 2 commits March 21, 2024 16:50
@csviri
improve: reconcile-condition naming
a2812e4 
Signed-off-by: Attila Mészáros <[email protected]>
@csviri
additional renames
88787d9 
Signed-off-by: Attila Mészáros <[email protected]>
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2024
@csviri csviri self-assigned this Mar 21, 2024
@csviri csviri requested a review from metacosm March 21, 2024 16:00
@csviri
Copy link
Collaborator Author

csviri commented Mar 21, 2024

  • we might want to discuss still if we want this, if yes however, would be probably rename also:
    ReadyPostCondition -> ReadyCondition
    DeletePostCondition -> DeleteCondition

@metacosm @jcechace what do you think?

@csviri csviri linked an issue Mar 21, 2024 that may be closed by this pull request
Change Naming of reconcilePrecondition for v5 #1926
Closed
@csviri
format
8f19ae6 
Signed-off-by: Attila Mészáros <[email protected]>
@jcechace
Copy link
Contributor

IDK .. personally I like the pre/post-codition naming as it makes it implicit at what stage the condition is evaluated.

@csviri
Copy link
Collaborator Author

csviri commented Mar 25, 2024

IDK .. personally I like the pre/post-codition naming as it makes it implicit at what stage the condition is evaluated.

yes, that is why I'm hesitating. Just we had a few misunderstandings mainly with reconcilePreCondition, since it also deletes the resource if the condition does not hold.

@csviri
Copy link
Collaborator Author

csviri commented Mar 25, 2024

But more and more I think about it, it might not be worth it.

@metacosm
Copy link
Collaborator

IDK .. personally I like the pre/post-codition naming as it makes it implicit at what stage the condition is evaluated.

yes, that is why I'm hesitating. Just we had a few misunderstandings mainly with reconcilePreCondition, since it also deletes the resource if the condition does not hold.

I don't think that renaming the condition to something else (unless that becomes really a mouthful like reconcileOrDeletePreCondition) would help. We already have javadoc for the meaning of these conditions. Maybe we could improve them but I'm not even sure we could be much clearer.

@csviri
Copy link
Collaborator Author

csviri commented Mar 25, 2024

kk, agree, will close this issue and the PR for now.

@csviri csviri closed this Mar 25, 2024
@metacosm metacosm deleted the reconcile-condition branch March 25, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metacosm metacosm Awaiting requested review from metacosm

Assignees

@csviri csviri

Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change Naming of reconcilePrecondition for v5
3 participants
@csviri @jcechace @metacosm